fix(session)!: serve channels and graphics that Windows moves onto a tunnel - #2008
Conversation
GitHub Actions
Error
github-actions
Bot
added
breaking-change
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change is sound. Shared process_create/process_close keep the TCP path's behavior intact while letting tunnels carry Create/Close PDUs after Soft-Sync; created channels bind to their arrival tunnel, declined creates are still answered on it, and the reply_tunnel fallback covers the cases the Soft-Sync routing table cannot. The active_stage refactor preserves the drain/reset/full-refresh ordering, and the declared breaking signature change is updated in its only workspace caller; the new test covers refusal before Soft-Sync, accepted and declined tunnel creates, data routing, and close. One low-severity robustness gap survives review: once every tunnel-bound channel closes, the unbinding flips reliable_udp_dvc_tunnel_in_use() false while the transport stays open, so later tunnel payloads (including the Create Request that could re-bind a channel) are buffered forever instead of failing as the pre-change error path did.
Reduced coverage: optional reviewer code-compressor was unavailable.
| DrdynvcServerPdu::Close(close) => { | ||
| debug!(?tunnel_type, "Got DVC Close PDU on a multitransport tunnel: {close:?}"); | ||
| let channel_id = close.channel_id(); | ||
| let messages = self.process_close(channel_id); | ||
| Ok(DvcMessageBatch::new(channel_id, messages)) | ||
| } |
There was a problem hiding this comment.
[skeptical] Closing the last tunnel-bound channel stalls tunnel traffic permanently with no recovery path — low 🟡 — The shared process_close removes the tunnel binding, so after the server closes every channel bound to the reliable UDP tunnel, has_channels_on_tunnel (and reliable_udp_dvc_tunnel_in_use) returns false while the UDP transport stays open. In rdp.rs the udp_payload arm then stores tunnel payloads in pending_udp_payload, which the loop top drains only while the tunnel is in use; the later Create Request that could re-bind a channel is itself such a payload, and a duplicate Soft-Sync request is rejected, so the state can never recover and tunnel traffic is silently buffered instead of failing loudly like the pre-change error path. The same unbinding now also happens for server Closes arriving over TCP. The code chain is verified; whether a Windows host actually closes all tunnel channels and reuses the tunnel within one session is unverified, so severity stays low.
There was a problem hiding this comment.
Fixed in 963e3cf. The DRDYNVC client now records the tunnels its Soft-Sync response switched to (DrdynvcClient::switched_to_tunnel), and ActiveStage::reliable_udp_dvc_tunnel_in_use uses that instead of the current channel bindings. The tunnel therefore stays in use after its last channel closes, and a later Create Request on it is processed. process_tunnel now also rejects traffic on a tunnel the response did not switch to. Covered by the new dvc::client::tunnel_stays_in_use_after_its_channels_close and by a check added to active_stage_exposes_and_validates_soft_sync_routing, which fails without the change.
GitHub Actions
Error
AKolenda
changed the title
|
On the overlap notice: #2007 and #2008 are complementary and merge cleanly in either order. #2007 decides which channels the Soft-Sync response binds when the request also lists channels the client declined. #2008 handles the Create, Close and graphics traffic that arrives on the tunnel after the switch. Against Windows 11 the graphics pipeline needs both to work over the tunnel. |
There was a problem hiding this comment.
The PR lets DRDYNVC handle Create and Close PDUs on Soft-Sync tunnels after the switch, tracks switched tunnels separately from per-channel bindings so the tunnel stays in use after its channels close, routes responses back on the arriving transport (including declined Creates), and drains the EGFX compositor for tunnel-carried data via helpers shared with the TCP path. The core fixes check out against the head and MS-RDPEDYC, with tests covering the new paths. Three low-severity findings published: post-switch UDP transport loss now always fails the session (documented trade-off, TCP fallback unreachable), the tunnel Create path skipping the caps-response recovery the TCP path has, and process_close duplicating close_channel. The unused-API nit was rejected as defensible, test-supported public API.
- [code-compressor] process_close re-implements close_channel instead of delegating to it — low 🟡 — crates/ironrdp-dvc/src/client.rs
process_close repeats close_channel (client.rs:311-315) line for line: remove the dynamic channel, drop the tunnel binding, and answer with a Close PDU only when the channel existed. Implementing it as a match over self.close_channel(channel_id) (Some => one-message Vec, None => empty) gives one implementation for the TCP and tunnel close paths, preventing drift on protocol-visible semantics; the trade-offs are log-only (client-initiated closes would also log) plus an unobservable flip in the order of the two map removals.
| self.x224_processor | ||
| .get_svc_processor::<DrdynvcClient>() | ||
| .is_some_and(|drdynvc| drdynvc.has_channels_on_tunnel(SoftSyncTunnelType::RELIABLE_UDP)) | ||
| .is_some_and(|drdynvc| drdynvc.switched_to_tunnel(SoftSyncTunnelType::RELIABLE_UDP)) |
There was a problem hiding this comment.
[skeptical] Post-switch UDP transport loss always fails the session; TCP fallback unreachable — low 🟡 — reliable_udp_dvc_tunnel_in_use now reports true from the Soft-Sync switch onward regardless of bound channels, so the unchanged client failure branches (rdp.rs:3158 and the udp_payload=None arm at 3266-3270) return TransportFailure for the rest of the session when the UDP transport closes after a switch; the warn-and-continue-on-TCP path is now reachable only before the switch. The PR documents this, and failing is defensible: after a fallback the server could still send Create or graphics traffic on the tunnel that the client would never read, and TransportFailure triggers auto-reconnect. Still an availability trade-off on flaky links (full reconnect instead of degraded TCP); worth confirming intent or documenting a narrower policy in a follow-up.
There was a problem hiding this comment.
This is intended. After the Soft-Sync exchange, MS-RDPEDYC 3.3.5.3.1 requires the server to keep sending each switched channel's data on the tunnel, and nothing moves a channel back to DRDYNVC. If the client fell back to TCP after losing the tunnel, those channels, the graphics pipeline among them, would stay silent for the rest of the session. So the session fails with TransportFailure, which starts the client's auto-reconnect when the policy allows it. Before the switch, no channel depends on the tunnel yet, and the client keeps running on TCP. I've kept that policy in this PR.
| DrdynvcServerPdu::Create(create_request) => { | ||
| debug!( | ||
| ?tunnel_type, | ||
| "Got DVC Create Request PDU on a multitransport tunnel: {create_request:?}" | ||
| ); | ||
| let channel_id = create_request.channel_id(); | ||
| let (created, messages) = self.process_create(create_request)?; | ||
| if created { | ||
| self.tunnel_channels.insert(channel_id, tunnel_type); | ||
| } | ||
| Ok(DvcMessageBatch::new(channel_id, messages)) | ||
| } |
There was a problem hiding this comment.
[skeptical] Tunnel Create path skips the capabilities-response recovery the TCP path performs — low 🟡 — The TCP Create arm synthesizes a Capabilities Response when a Create Request arrives before the caps handshake (client.rs:548-554), defending against out-of-order server PDUs. process_tunnel's Create arm calls process_create with no such check, so a server that sends the Soft-Sync Request before capabilities and then a Create Request on the tunnel gets only a Create Response; cap_handshake_done stays false and a later TCP Create would emit a late caps response. Handling the precondition in the tunnel path (or asserting it) keeps the two Create paths consistent.
There was a problem hiding this comment.
Fixed in 0c520e8. A Create Request that arrives on a tunnel before the capabilities exchange is now refused. The TCP path recovers by sending a Capabilities Response on DRDYNVC first. Capabilities PDUs are not exchanged on a tunnel, so the tunnel path cannot do the same. Soft-Sync does not depend on the DVC version, so the Soft-Sync request can't rule this case out either. Covered by the new dvc::client::tunnel_refuses_a_create_request_before_the_capabilities_exchange; the two existing tunnel tests now run the capabilities exchange first.
github-actions
Bot
added
ai-reviewed/2
…#2007) Windows lists every dynamic channel it intends to move in its Soft-Sync request, including the ones the client declined with NO_LISTENER. Against a Windows 11 host the request lists channels 2, 6, 7, 8, 9, 10, 11 and 12 (CoreInput, MouseCursor, Graphics, Video, Geometry, ...), and only channel 7, the graphics pipeline, is open. `process_soft_sync_request` dropped a whole channel list as soon as one ID in it was not open. The tunnel was then never switched, and the channels the client had opened stayed on TCP while the server was already sending them on the tunnel (MS-RDPEDYC 3.2.5.3.1). Unopened channels are now skipped one by one, and the tunnel is switched for the rest. ## Testing - New `dvc::client::soft_sync_skips_channels_the_client_did_not_open` in `ironrdp-testsuite-core`. - Live, against a Windows 11 host over RDP-UDP version 2, with the viewer built from a branch that also carries the tunnel and client PRs of this series: the Soft-Sync request above now switches the tunnel, and the graphics pipeline moves onto it. ## Checks - `cargo fmt --all -- --check` - `cargo clippy --workspace --all-targets --features helper,__bench --locked -- -D warnings` - `cargo test --locked -p ironrdp-testsuite-core -p ironrdp-testsuite-extra`, plus the lib tests of the crates touched here - `cargo test --workspace --locked` on a branch that merges this PR with the other Windows interop PRs from this series - `typos` on the changed files ## Series These PRs port the Windows interop fixes and Linux backends from a downstream IronRDP fork, so the fork can be retired. Each one is based on `master` and can be reviewed and merged on its own. I also checked that all of them merge cleanly together in this order. - #2007 fix(dvc): Soft-Sync tunnel with declined channels - #2008 fix(session)!: channels and graphics on the tunnel - #2009 fix(rdpeudp): auto-detect on the tunnel - #2010 fix(graphics)!: SRL streams from Windows - #2011 fix(egfx): bitmap cache across ResetGraphics - #2012 feat(session): bandwidth measurements during the session - #2013 feat(client): graphics pipeline and RDP-UDP version options - #2014 fix(client): resize reconnects on the graphics pipeline - #2015 feat(client): transport event - #2016 feat(cliprdr): Linux clipboard backend - #2017 feat(rdpdr): printer on Linux and macOS Co-authored-by: AKolenda <testedemail2222@gmail.com>
…o a tunnel Once Soft-Sync has moved dynamic channels onto the reliable UDP tunnel, Windows keeps using the tunnel for more than channel data, and two gaps kept the graphics pipeline from working there: - dvc: after Soft-Sync, Windows opens new channels (AUDIO_PLAYBACK_DVC, RDS::Input) with Create Request PDUs sent on the tunnel, and closes them there too. `process_tunnel` accepted only data PDUs, so the first Create Request failed the session. Create and Close are now handled on a tunnel once Soft-Sync has completed. A channel created on a tunnel is bound to it, and the client sends every response back on the tunnel the request came from, including the NO_LISTENER response for a declined channel, which the Soft-Sync routing table cannot place. - session: `process_dvc_tunnel` never drained the EGFX compositor, so frames decoded from tunnel data never reached the framebuffer and the window stayed black. It now takes the image, follows a pending ResetGraphics, composites completed frames and records their damage the same way `process` does for TCP-carried DVC data, and returns the graphics updates next to the message batch. BREAKING CHANGE: `ActiveStage::process_dvc_tunnel` takes the decoded image and returns the graphics updates along with the DVC message batch.
The reliable UDP tunnel counted as in use only while a channel was bound to it. Once the server closed the last bound channel, the client stopped processing tunnel payloads and buffered them, including a later Create Request that would bind a new channel, so the tunnel stalled for good. Record the tunnels the Soft-Sync response switched to and treat those as in use for the rest of the session.
The Close handler repeated `close_channel`. It now calls it, so the TCP and tunnel close paths share one implementation. `close_channel` drops a channel's tunnel binding before checking whether the channel is open, as the handler did.
…ange A Create Request that arrives on TCP before the capabilities exchange is answered with a Capabilities Response first. Capabilities PDUs are not exchanged on a tunnel, so a Create Request on a tunnel before the exchange was processed without one and left the handshake open. The tunnel now refuses it.
963e3cf to
0c520e8
Compare
github-actions
Bot
added
needs-review
|
On the review's first finding, that |
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you! LGTM
bfd16a2
into
Devolutions:master
Once Soft-Sync has moved dynamic channels onto the reliable UDP tunnel, Windows keeps using the tunnel for more than channel data. Two gaps kept the graphics pipeline from working there.
dvc: channel lifecycle on the tunnel. After Soft-Sync, Windows opens new channels with Create Request PDUs sent on the tunnel (Video::Control, Video::Data, Geometry and AUDIO_PLAYBACK_DVC on the host I tested), and closes them there too.
process_tunnelaccepted only data PDUs, so the first Create Request failed the session. Create and Close are now handled on a tunnel once the Soft-Sync response has switched to it:close_channel, so it also removes the channel's tunnel binding.DrdynvcClient::switched_to_tunnelreports this, andActiveStage::reliable_udp_dvc_tunnel_in_usenow uses it instead of the current channel bindings. Otherwise, closing the last bound channel would make the client stop reading the tunnel, so a later Create Request on it would never be processed. If the UDP transport closes after the switch, the session fails as it already did while channels were bound.session: graphics received on the tunnel.
process_dvc_tunnelnever drained the EGFX compositor, so frames decoded from tunnel data never reached the framebuffer and the window stayed black. It now takes the image, follows a pending ResetGraphics, composites completed frames and records their damage the same wayprocessdoes for TCP-carried DVC data, and returns the graphics updates next to the message batch. The drain and the full-refresh widening after an output reset move into two private helpers shared by both paths.Breaking change
ActiveStage::process_dvc_tunnel(&mut self, image, tunnel_type, payload)now takes the decoded image and returns(DvcMessageBatch, Vec<ActiveStageOutput>).ironrdp-clientis the only caller in the workspace and is updated.Testing
dvc::client::channels_created_on_a_tunnel_are_bound_to_itinironrdp-testsuite-core, covering refusal before Soft-Sync, an accepted and a declined Create on the tunnel, data routing afterwards, and Close on the tunnel.dvc::client::tunnel_stays_in_use_after_its_channels_close.dvc::client::tunnel_refuses_a_create_request_before_the_capabilities_exchange.active_stage_exposes_and_validates_soft_sync_routingunit test is updated for the new signature, and checks that the tunnel stays in use after the server closes every channel on it.Checks
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --features helper,__bench --locked -- -D warningscargo test --locked -p ironrdp-testsuite-core -p ironrdp-testsuite-extra, plus the lib tests of the crates touched herecargo test --workspace --lockedon a branch that merges this PR with the other Windows interop PRs from this seriestyposon the changed filesSeries
These PRs port the Windows interop fixes and Linux backends from a downstream IronRDP fork, so the fork can be retired. Each one is based on
masterand can be reviewed and merged on its own. I also checked that all of them merge cleanly together in this order.